Allow DevelopmentServer testing with internal and external plugins together #23100
Conversation
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
|
I think this would be a great feature to add for developers. Reopened and added staleignore. I will try to review and help towards merge. |
mosabua
added
stale-ignore
xkrogen
force-pushed
the
xkrogen/hybrid-development-plugins-provider
branch
from
23a0e5c
to
903aaff
Compare
|
Rebased! Should be ready for review again :) |
testing/trino-server-dev/src/main/java/io/trino/server/HybridDevelopmentPluginsProvider.java
Outdated
Show resolved
Hide resolved
xkrogen
force-pushed
the
xkrogen/hybrid-development-plugins-provider
branch
from
629b15e
to
2c30807
Compare
|
Code looks good to me. Just need to get docs updated and do some testing. Anybody else wants to chime in about their testing of this or other aspects @nineinchnick @martint @electrum ? |
xkrogen
force-pushed
the
xkrogen/hybrid-development-plugins-provider
branch
from
85f200a
to
8379005
Compare
|
Can you squash commits @xkrogen ? |
…gether Co-authored-by: Pratham Desai <[email protected]>
xkrogen
force-pushed
the
xkrogen/hybrid-development-plugins-provider
branch
from
8379005
to
b1e718d
Compare
I thought the point of the Anyway I have squashed, just want to better understand this for next time. |
| @@ -34,3 +34,7 @@ product-test-reports | |||
| **/dependency-reduced-pom.xml | |||
| core/trino-web-ui/src/main/resources/webapp/dist/ | |||
| .node | |||
|
|
|||
| # Local plugins added for testing should be excluded | |||
| testing/trino-server-dev/etc/plugin/* | |||
There was a problem hiding this comment.
Isn't this redundant with testing/trino-server-dev/etc/plugin/.gitignore?
There was a problem hiding this comment.
Yes! This was added before I added the nested .gitignore. Thanks for catching it.
| @Override | ||
| public void loadPlugins(Loader loader, ClassLoaderFactory createClassLoader) | ||
| { | ||
| developmentPluginsProvider.loadPlugins(loader, createClassLoader); |
There was a problem hiding this comment.
This looks ok to me, but I wonder if we can improve the developer experience here. Coping plugins is one extra step that could be avoided - if the DevelopmentPluginsProvider was able to load a plugin from a directory, then we could add directories to the plugin.bundles list.
There was a problem hiding this comment.
Hm I see what you're saying. Currently, plugins.bundle supports pom.xml/*.pom files as well as Maven coordinates, and we could also add support for directories. It's a good idea! I can take a look at this.
Description
This PR makes it possible to use the local
DevelopmentServerwith both "internal" plugins (those within thetrinorepo) as well as "external" plugins that originate from outside of the repository. A newHybridDevelopmentPluginsProvideris added which loads plugins both fromplugin.dirand fromplugin.bundles, and this is now used by default inDevelopmentServer. No additional plugins are loaded by default, but any plugin directories dropped intotesting/trino-server-dev/etc/pluginwill be picked up automatically.For now this is added as an entirely new
PluginsProviderwhich combines the behavior of the development and server plugins providers, but I would also be happy to structure it such thatDevelopmentPluginsProviderjust does this internally (by delegating toServerPluginsProvider). Feedback is welcomed.Additional context and related issues
In #6834, the whole dev server was refactored. Previously, it was possible to use
plugin.diras well asplugin.bundlessimultaneously, but after the refactor, these were moved to two separate plugin loaders (ServerPluginsProviderandDevelopmentPluginsProviders, respectively).DevelopmentServeronly usesDevelopmentPluginsProvider, so it can load plugins from POM, but cannot load plugins from a directory, making it challenging to leverage theDevelopmentServerto test other plugins. This can be really useful while working on third party plugins that live outside of the Trino repo, and in particular, to test interactions between plugins, such as redirection, or inspecting the events received by a third-party event listener when working with an internal catalog.Release notes
(X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: